Skip to content

do not propagate test log to CLI#695

Merged
lukaspie merged 3 commits intomasterfrom
do-not-propagate-logger-in-testing
Aug 13, 2025
Merged

do not propagate test log to CLI#695
lukaspie merged 3 commits intomasterfrom
do-not-propagate-logger-in-testing

Conversation

@lukaspie
Copy link
Collaborator

@lukaspie lukaspie commented Aug 13, 2025

Currently, the whole read_nexus output gets logged to the CLI when pytest is run. In the GitHub Actions interface, this clutters the output and it can be fairly hard to find the actual issues if the tests fail.

Here, we use logger.propagate = False so that the log is stored in the files, but not in the CLI output.

Additionally, changes to the actual output of the log comparison is made. So far, if the line count mismatched, we would not get any info which lines are different. Now, we log those lines that are unique in both files, making it easier to understand how the two logs are different.

@lukaspie lukaspie requested a review from RubelMozumder August 13, 2025 11:51
Copy link
Collaborator

@rettigl rettigl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test that this actually does work and does the right thing when tests fail in the different fashions? Passing tests here do not execute the changed code...

@lukaspie
Copy link
Collaborator Author

Did you test that this actually does work and does the right thing when tests fail in the different fashions? Passing tests here do not execute the changed code...

See #695 (comment) for an example where the number of log lines are different (case 1 above)

In addition, I also modified one line in another pynxtools-xps test (to trigger case 2) and I get the result that we had seen so far. We get the same result as we got before:

AssertionError: Log files are different at line 5657:
generated: DEBUG - value: 2023-03-08T15:30:27+01:00
reference: DEBUG - value: 2023-03-08T15:30:27+00:00

So I think that covers both use cases.

Copy link
Collaborator

@RubelMozumder RubelMozumder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not that much difference, at least experiments are saying that.
Let's go with it and see how it affects us.

@lukaspie
Copy link
Collaborator Author

@RubelMozumder @rettigl thanks for the feedback. Just FYI, I slightly changed the logging functionality to match that of the case where only some lines are different. But that does not affect the functionality here. Merging now.

@lukaspie lukaspie merged commit 9fc61b0 into master Aug 13, 2025
16 checks passed
@lukaspie lukaspie deleted the do-not-propagate-logger-in-testing branch August 13, 2025 14:11
@rettigl
Copy link
Collaborator

rettigl commented Aug 13, 2025

There are still 1000s of log lines here: https://github.com/FAIRmat-NFDI/pynxtools/actions/runs/16939887832/job/48005872843
Wasn't it the idea to remove these from the test logs?

@rettigl
Copy link
Collaborator

rettigl commented Aug 13, 2025

There are still 1000s of log lines here: https://github.com/FAIRmat-NFDI/pynxtools/actions/runs/16939887832/job/48005872843 Wasn't it the idea to remove these from the test logs?

Okay, it's a lot less than before, but still a lot coming from the Nomad parsing. Maybe one disables the logging propagation there as well?

@lukaspie
Copy link
Collaborator Author

There are still 1000s of log lines here: FAIRmat-NFDI/pynxtools/actions/runs/16939887832/job/48005872843 Wasn't it the idea to remove these from the test logs?

Okay, it's a lot less than before, but still a lot coming from the Nomad parsing. Maybe one disables the logging propagation there as well?

I was thinking about that, but then the NOMAD parsing is essentially silent. At least now the number of log lines is small enough that the GitHub UI can display the actual error (previously it was often just loading and loading). I am fine with suppressing the NOMAD logs or maybe we only keep those that are at least a warning?

@rettigl
Copy link
Collaborator

rettigl commented Aug 13, 2025

maybe we only keep those that are at least a warning?

That sounds like a good idea. Right now, one would not even find those because of all the output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants